From b34408aff76707b9e0ddd5130044f9296927043f Mon Sep 17 00:00:00 2001 From: Jyrki Gadinger Date: Wed, 21 May 2025 15:13:13 +0200 Subject: [PATCH] fix(propagator): touch folder paths if permissions changed Extra safeguard to ensure that the usage of `FileSystem::setFolderPermissions` won't start a new sync run if FolderWatcher/inotify detects a change in the file metadata... Signed-off-by: Jyrki Gadinger --- src/libsync/owncloudpropagator.cpp | 12 +++-- test/syncenginetestutils.h | 2 +- test/testallfilesdeleted.cpp | 4 +- test/testblacklist.cpp | 2 +- test/testsyncengine.cpp | 72 ++++++++++++++++++++++++++++++ test/testsyncmove.cpp | 2 +- 6 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 5751911e1..bd597dee8 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1468,11 +1468,14 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) !_item->_remotePerm.hasPermission(RemotePermissions::CanAddFile) && !_item->_remotePerm.hasPermission(RemotePermissions::CanAddSubDirectories)) { try { - if (FileSystem::fileExists(propagator()->fullLocalPath(_item->_file))) { - FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_file), FileSystem::FolderPermissions::ReadOnly); + if (const auto fileName = propagator()->fullLocalPath(_item->_file); FileSystem::fileExists(fileName)) { + FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadOnly); + Q_EMIT propagator()->touchedFile(fileName); } if (!_item->_renameTarget.isEmpty() && FileSystem::fileExists(propagator()->fullLocalPath(_item->_renameTarget))) { - FileSystem::setFolderPermissions(propagator()->fullLocalPath(_item->_renameTarget), FileSystem::FolderPermissions::ReadOnly); + const auto fileName = propagator()->fullLocalPath(_item->_renameTarget); + FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadOnly); + Q_EMIT propagator()->touchedFile(fileName); } } catch (const std::filesystem::filesystem_error &e) @@ -1495,10 +1498,11 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) } } else { try { - const auto permissionsChangeHelper = [] (const auto fileName) + const auto permissionsChangeHelper = [this] (const auto fileName) { qCDebug(lcDirectory) << fileName << "permissions changed: old permissions" << static_cast(std::filesystem::status(fileName.toStdWString()).permissions()); FileSystem::setFolderPermissions(fileName, FileSystem::FolderPermissions::ReadWrite); + Q_EMIT propagator()->touchedFile(fileName); qCDebug(lcDirectory) << fileName << "applied new permissions" << static_cast(std::filesystem::status(fileName.toStdWString()).permissions()); }; diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h index 44ed5edda..2aaa4f565 100644 --- a/test/syncenginetestutils.h +++ b/test/syncenginetestutils.h @@ -575,7 +575,7 @@ public: [[nodiscard]] OCC::SyncJournalDb &syncJournal() const { return *_journalDb; } [[nodiscard]] FakeQNAM* networkAccessManager() const { return _fakeQnam; } - FileModifier &localModifier() { return _localModifier; } + DiskFileModifier &localModifier() { return _localModifier; } FileInfo &remoteModifier() { return _fakeQnam->currentRemoteState(); } FileInfo currentLocalState(); diff --git a/test/testallfilesdeleted.cpp b/test/testallfilesdeleted.cpp index 14ca3a647..3fbf96f73 100644 --- a/test/testallfilesdeleted.cpp +++ b/test/testallfilesdeleted.cpp @@ -76,7 +76,7 @@ private slots: fakeFolder.syncEngine().journal()->clearFileTable(); // That's what Folder is doing }); - auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : fakeFolder.localModifier(); + auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : static_cast(fakeFolder.localModifier()); const auto childrenKeys = fakeFolder.currentRemoteState().children.keys(); for (const auto &key : childrenKeys) { modifier.remove(key); @@ -118,7 +118,7 @@ private slots: callback(false); }); - auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : fakeFolder.localModifier(); + auto &modifier = deleteOnRemote ? fakeFolder.remoteModifier() : static_cast(fakeFolder.localModifier()); const auto childrenKeys = fakeFolder.currentRemoteState().children.keys(); for (const auto &key : childrenKeys) { modifier.remove(key); diff --git a/test/testblacklist.cpp b/test/testblacklist.cpp index f1445689c..ffce7f6c1 100644 --- a/test/testblacklist.cpp +++ b/test/testblacklist.cpp @@ -46,7 +46,7 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); ItemCompletedSpy completeSpy(fakeFolder); - auto &modifier = remote ? fakeFolder.remoteModifier() : fakeFolder.localModifier(); + auto &modifier = remote ? fakeFolder.remoteModifier() : static_cast(fakeFolder.localModifier()); int counter = 0; const QByteArray testFileName = QByteArrayLiteral("A/new"); diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 5a53904a5..7ab6431d2 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -2345,6 +2345,78 @@ private slots: QCOMPARE(completeSpy.findItem(fileWithoutSpaces6)->_status, SyncFileItem::Status::Success); QCOMPARE(completeSpy.findItem(extraFileNameWithoutSpaces)->_status, SyncFileItem::Status::Success); } + + void testTouchedFilesWhenChangingFolderPermissionsDuringSync() + { + FakeFolder fakeFolder{FileInfo{}}; + fakeFolder.localModifier().mkdir("directory"); + fakeFolder.localModifier().mkdir("directory/subdir"); + fakeFolder.remoteModifier().mkdir("directory"); + fakeFolder.remoteModifier().mkdir("directory/subdir"); + + // perform an initial sync to ensure local and remote have the same state + QVERIFY(fakeFolder.syncOnce()); + + QStringList touchedFiles; + + // syncEngine->_propagator is only set during a sync, which doesn't work with QSignalSpy :( + connect(&fakeFolder.syncEngine(), &SyncEngine::started, this, [&]() { + // at this point we have a propagator to connect signals to + connect(fakeFolder.syncEngine().getPropagator().get(), &OwncloudPropagator::touchedFile, this, [&touchedFiles](const QString& fileName) { + touchedFiles.append(fileName); + }); + }); + + const auto syncAndExpectNoTouchedFiles = [&]() { + touchedFiles.clear(); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(touchedFiles.size(), 0); + }; + + // when nothing changed expect no files to be touched + syncAndExpectNoTouchedFiles(); + + // when the remote etag of a subsubdir changes expect the parent+subdirs to be touched + fakeFolder.remoteModifier().findInvalidatingEtags("directory/subdir"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(touchedFiles.size(), 2); + QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory/subdir").fileName())); + QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName())); + + // nothing changed again, expect no files to be touched + syncAndExpectNoTouchedFiles(); + + // when subdir folder permissions change, expect the parent to be touched + touchedFiles.clear(); + fakeFolder.remoteModifier().find("directory")->permissions = RemotePermissions::fromServerString("S"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(touchedFiles.size(), 1); + QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName())); + + // another sync without changes, expect no files to be touched + syncAndExpectNoTouchedFiles(); + + // remote etag of the subdir changed, expect the parent to be touched + touchedFiles.clear(); + fakeFolder.remoteModifier().findInvalidatingEtags("directory"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(touchedFiles.size(), 1); + QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("directory").fileName())); + + // same as usual, expect no files to be touched + syncAndExpectNoTouchedFiles(); + + // remote rename of the subdir folder, expect the new name to be touched + touchedFiles.clear(); + fakeFolder.remoteModifier().rename("directory", "renamedDirectory"); + QVERIFY(fakeFolder.syncOnce()); + qDebug() << touchedFiles; + QCOMPARE_GT(touchedFiles.size(), 1); + QVERIFY(touchedFiles.contains(fakeFolder.localModifier().find("renamedDirectory").fileName())); + + // last sync without changes, expect no files to be touched + syncAndExpectNoTouchedFiles(); + } }; QTEST_GUILESS_MAIN(TestSyncEngine) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 04d9456e0..cb73f243f 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -812,7 +812,7 @@ private slots: { QFETCH(bool, local); FakeFolder fakeFolder { FileInfo::A12_B12_C12_S12() }; - auto &modifier = local ? fakeFolder.localModifier() : fakeFolder.remoteModifier(); + auto &modifier = local ? static_cast(fakeFolder.localModifier()) : fakeFolder.remoteModifier(); modifier.mkdir("FolA"); modifier.mkdir("FolA/FolB"); -- 2.30.2